Audit Fixes for RLP library on Broadcaster Audit #6106

luiz-lvj · 2025-11-13T18:38:08Z

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

changeset-bot · 2025-11-13T18:38:13Z

🦋 Changeset detected

Latest commit: fe917b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

coderabbitai · 2025-11-13T18:43:10Z

Walkthrough

The RLP library in contracts/utils/RLP.sol was refactored with several changes: the encode(address) function now delegates to encode(uint256) instead of using inline assembly; readAddress removed explicit length validation and delegates to readUint256; readBytes documentation clarified about length validation handling; _decodeLength return signature changed from named variables to unnamed tuples; encode(bytes[] memory) received updated documentation about pre-encoded elements; and encode(Encoder) had formatting alignment adjustments.

Possibly related PRs

Add RLP library #5680: Directly modifies functions and signatures in contracts/utils/RLP.sol including encode(address), readAddress, and _decodeLength.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Audit Fixes for RLP library on Broadcaster Audit' directly relates to the changeset, which contains modifications to the RLP.sol utility library addressing audit findings.
Description check	✅ Passed	The description contains a PR checklist related to standard merge requirements (Tests, Documentation, Changeset entry), which is relevant to the pull request process and changeset context.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6308fdc and 55b33a0.

📒 Files selected for processing (1)

contracts/utils/RLP.sol (4 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Redirect rules - solidity-contracts
GitHub Check: Header rules - solidity-contracts
GitHub Check: coverage
GitHub Check: tests-foundry
GitHub Check: slither
GitHub Check: tests-upgradeable
GitHub Check: tests
GitHub Check: halmos
GitHub Check: Pages changed - solidity-contracts

contracts/utils/RLP.sol

Amxx · 2025-11-13T18:49:53Z

contracts/utils/RLP.sol

-        assembly ("memory-safe") {
-            result := mload(0x40)
-            mstore(result, 0x15) // length of the encoded data: 1 (prefix) + 0x14 (address)
-            mstore(add(result, 0x20), or(shl(248, 0x94), shl(88, input))) // prefix (0x94 = SHORT_OFFSET + 0x14) + input
-            mstore(0x40, add(result, 0x35)) // reserve memory
-        }


This encoding is not consistent with how the ethereum ecosystem does things.

If you try to use ethers.js to do encode an address that had zeros, you'll get them encoded

> ethers.encodeRlp(ethers.ZeroAddress); '0x940000000000000000000000000000000000000000'

Note that this difference is important in the context of operations like getCreateAddress. If the address prefix zeros are removed, you'd get an invalid address prediction.

I'll add unit tests to show these issues.

I mentioned to the auditors that it makes sense to include leading zeroes in addresses because there's no definition of an address in RLP. My thinking about this is that leading zeroes in an address are not necessarily "leading" because they constitute relevant information for the address.

Amxx · 2025-11-13T19:20:13Z

Added tests that demonstrate the breaking effect of these changes. You can see on #6107 that these tests pass on master.

If someone wants to encode an Address without the leading zeros, they can manually do the casting to uint256 and then call the corresponding encode function. However I believe this should not be the default encoding. In particular, predicting the addresses of contracts created using the CREATE mechanism requires the addresses to have the prefix zeros. IMO the library must offer a function that behaves that way, and I think the encode(address) is the one.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

…mxx/openzeppelin-contracts into testing/rlp-encoding-addresses

…Prefixes

ernestognw · 2025-11-14T22:20:44Z

contracts/utils/RLP.sol

        assembly ("memory-safe") {
            result := mload(0x40)
-            mstore(result, 0x15) // length of the encoded data: 1 (prefix) + 0x14 (address)
+            mstore(result, 21) // length of the encoded data: 1 (prefix) + 0x14 (address)


Isn't this against our guidelines?

Numeric literals should use appropriate formats based on their purpose to improve code readability:

Memory-related operations (use hexadecimal):

Memory locations: mload(64) → mload(0x40)

Memory offsets: mstore(add(ptr, 32), value) → mstore(add(ptr, 0x20), value)

Memory lengths: keccak256(ptr, 85) → keccak256(ptr, 0x55)

ernestognw · 2025-11-14T22:24:27Z

.changeset/slow-lemons-rest.md

I don't think we need a changeset since we're not changing functionality

luiz-lvj added 4 commits November 13, 2025 15:17

fix L-03

5800339

fix N-06

47c8048

fix N-07

78f643d

fix N-09

55b33a0

luiz-lvj requested a review from a team as a code owner November 13, 2025 18:38

luiz-lvj requested review from a team and ernestognw and removed request for a team November 13, 2025 18:38

add changeset

e0a6e6b

coderabbitai bot reviewed Nov 13, 2025

View reviewed changes

contracts/utils/RLP.sol Outdated Show resolved Hide resolved

Amxx reviewed Nov 13, 2025

View reviewed changes

Test encoding of addresses with leading zeros

f437257

Amxx mentioned this pull request Nov 13, 2025

Test encoding of addresses with leading zeros #6107

Open

Merge branch 'testing/rlp-encoding-addresses' into audit-fixes/RLP

0ac8a92

Amxx and others added 8 commits November 14, 2025 10:12

Update test/utils/RLP.test.js

60b70b7

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

add fuzzing test

6a86e31

Merge branch 'testing/rlp-encoding-addresses' of https://github.com/A…

08d217f

…mxx/openzeppelin-contracts into testing/rlp-encoding-addresses

Merge branch 'testing/rlp-encoding-addresses' into audit-fixes/RLP

5f11e08

revert back to address

1992bed

remove changeset

be65eb8

update changeset

8f2dd16

Solve: Inconsistent Integer Base in Inline Assembly When Setting RLP …

61b695f

…Prefixes

luiz-lvj requested a review from Amxx November 14, 2025 15:11

test string qith leading zeros

fe917b7

ernestognw reviewed Nov 14, 2025

View reviewed changes

.changeset/slow-lemons-rest.md

Copy link

Member

ernestognw Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a changeset since we're not changing functionality

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Audit Fixes for RLP library on Broadcaster Audit #6106

Audit Fixes for RLP library on Broadcaster Audit #6106

luiz-lvj commented Nov 13, 2025

Uh oh!

changeset-bot bot commented Nov 13, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 13, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Amxx Nov 13, 2025

Uh oh!

ernestognw Nov 14, 2025

Uh oh!

Amxx commented Nov 13, 2025 •

edited

Loading

Uh oh!

ernestognw Nov 14, 2025

Uh oh!

ernestognw Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Audit Fixes for RLP library on Broadcaster Audit #6106

Are you sure you want to change the base?

Audit Fixes for RLP library on Broadcaster Audit #6106

Conversation

luiz-lvj commented Nov 13, 2025

PR Checklist

Uh oh!

changeset-bot bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

coderabbitai bot commented Nov 13, 2025

Walkthrough

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Amxx Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

ernestognw Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Amxx commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ernestognw Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

ernestognw Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Nov 13, 2025 •

edited

Loading

Amxx commented Nov 13, 2025 •

edited

Loading